Skip to content

[2단계 - 나만의 유튜브 강의실] 유세지(김용래) 미션 제출합니다.#140

Merged
EungyuCho merged 37 commits into
woowacourse:usagenessfrom
usageness:step2
Mar 21, 2022
Merged

[2단계 - 나만의 유튜브 강의실] 유세지(김용래) 미션 제출합니다.#140
EungyuCho merged 37 commits into
woowacourse:usagenessfrom
usageness:step2

Conversation

@usageness
Copy link
Copy Markdown

@usageness usageness commented Mar 19, 2022

안녕하세요 아사님! 좋은 주말입니다 😊

이번 step2는 아래와 같은 요소들에 집중해서 진행해보았어요.
프로그램의 동작은 데모 페이지를 확인해보시기 바랄게요!

죄송하지만 크루분들은... 검색 버튼 한 번만 눌러주시기...ㅠㅠ

  • 우선 프로젝트의 요구사항을 모두 완료하기
  • 고통스럽지만 끝까지 TDD로 진행해보기
  • 내가 유저라면 필요할 것 같은 기능들을 추가해보기
  • 최대한 높은 성능을 유지하는 페이지를 만들어보기

특히 성능면에서는 DOM을 다시 그리거나, 불러오는 작업들을 최대한 지양하면서 진행하였습니다!
이번 step2도 잘 부탁드릴게요 👍👍

프로젝트 구조도

image

- constants.js 의 DEVELOP_MODE 에 boolean 값을 주어 mockdata 사용 여부
  조정 가능
- localStorage를 확인하여 비어있는 경우에만 안내 문구 표시
- class-methods-use-this: 0 추가
- 짝 테스트 설명도 함께 변경 : "초기 화면에서" -> "볼 영상 섹션에서"
- fetch error 핸들링 추가
Copy link
Copy Markdown

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 세지님~ 주말 잘 보내시고 계신가요? 😄

step2 요구사항 구현하시느라 고생 많으셨어요! 구조도까지 추가해주셨네요 ㅎㅎ

Comment

  • TDD를 시도해주셔서 E2E 테스트케이스가 꼼꼼하고 검증하신 플로우가 적절한 범위로 테스트하셨다고 느껴졌어요. 정말 고생 많으셨어요!
  • 접근지정자나 BEM, 코드 일관성을 지켜주시려고 노력해주신 부분이 코드에서 잘 보였어요. 👍
  • YoutubeApp이 너무 비대해진 느낌이 들었어요. 다음 미션에서는 하위 컴포넌트로 분리해봐도 좋을 것 같아요.(Custom Element로 분리해주신 크루분들이 계신데 깔끔하다고 느꼈어요.)

코드 코멘트도 확인부탁드릴게요. 추가적으로 궁금하신 점이 있다면 코멘트 부탁드려요!~
주말 잘 보내시고 다음 리뷰요청때 뵐게요! ⛱️

Cypress.Commands.add("closeSearchModal", () => {
cy.get(".dimmer").click({ force: true });
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전 PR에 테스트중에 searchWithNoKeyword 커맨드 추가에서 고칠 부분이 있어요
assert가 안나오는 문제가 있어서 이렇게 고쳐봅시다 👀

cy.get("#search-button").click().then(() => {
    expect(alertStub).to.be.calledWith(ERROR_MESSAGE.SEARCH_INPUT_IS_EMPTY);
  });

Copy link
Copy Markdown
Author

@usageness usageness Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗... 다음 동작을 click() 안에 넣어버렸네요 😭😭 수정했습니다!

4d93220

Comment on lines +50 to +52
console.log(text);
expect(text).to.contains("정말로 삭제하시겠습니까?");
return confirmButtonClick;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return은 안해주셔도 괜찮아보여요!

Suggested change
console.log(text);
expect(text).to.contains("정말로 삭제하시겠습니까?");
return confirmButtonClick;
expect(text).to.contains("정말로 삭제하시겠습니까?");

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 문에 boolean 값을 넣어주는걸로 comfirm창의 확인 버튼과 취소 버튼을
누를 수 있다고 해서 인자로 받아 넣어주었었는데 혹시 제가 잘못 알고 있었던걸까요?
리턴을 지워주니 오류가 나기에 다시 여쭈어봅니다 😭

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 문에 boolean 값을 넣어주는걸로 comfirm창의 확인 버튼과 취소 버튼을 누를 수 있다고 해서 인자로 받아 넣어주었었는데 혹시 제가 잘못 알고 있었던걸까요? 리턴을 지워주니 오류가 나기에 다시 여쭈어봅니다 😭

cypress window:confirm에 return이 필요한걸 깜빡했네요 🙇
말씀하신대로 return을 넣어줘야 confirm창을 확인이나 취소할 수 있네요. 죄송합니다!

Comment thread src/js/templates.js Outdated
});
})
.join("");
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제 생각에는 View를 나눠주셨는데 굳이 템플릿을 분리할 필요가 있을지 의문이에요.
템플릿파일에 여러가지 View에 대한 마크업이 모두 저장되어있을 필요가 있을까요?

Copy link
Copy Markdown
Author

@usageness usageness Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음엔 View 별로 공통적으로 사용할 수 있는 템플릿이 있어서 분리하게 되었는데,
이후에 각자의 템플릿을 사용하도록 구조를 바꾸면서 남겨지게 되었네요...
이 부분은 View가 들고 있어도 좋을 것 같아 수정하였습니다!

de6a743

Comment thread src/js/utils/handleError.js Outdated
Comment on lines +1 to +11
const handleError = (errorMessage) => {
switch (errorMessage) {
case "Failed to fetch":
alert("인터넷 연결이 원활하지 않습니다. 잠시 후 다시 시도해주세요.");
break;
case "":
break;
}
};

export default handleError;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

youtube에 대한 에러 핸들링 부분이네요! 👍

유튜브 api에 종속적인 핸들링인만큼 util에 위치할만큼 분리의 필요성은 떨어져보이긴해서 catch문에서 switch문만 사용하거나 해당 api를 호출하는 파일 내부에 위치하는게 더 좋아보여요!

Copy link
Copy Markdown
Author

@usageness usageness Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed to fetch 메시지가 fetch 에서 반환하는 에러 메시지이다 보니 util이라고 착각했네요 😅
앞으로 추가될 수 있는 오류를 생각해봐도 유튜브 api에 종속적인 에러 핸들러 같습니다!
api의 catch문 내부로 이동해주었어요!

98c5a19

Comment thread src/js/VideoStorage.js Outdated
Comment on lines +13 to +15
return !this.getStorage().some(
(item) => item.isWatched === isWatchedVideoOnly
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some 활용 좋네요 👍

cy.closeSearchModal();
cy.get(".modal-container").should("be.not.visible");
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보고싶은 영상 찾기 모달창 안에서 검색된 영상이 이미 저장된 영상이라면 저장 버튼이 보이지 않아야 한다 테스트 케이스에서 에러가 발생해요.
cy.searchWithKeyword(searchKeyword)를 제거하면 이전에 검색한 상태에서 테스트를 진행해서 에러를 없앨 수 있어요 👀

Copy link
Copy Markdown
Author

@usageness usageness Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

같은 검색어로도 실제 검색 결과가 다를 수 있다는 점을 간과했네요 😭
불필요한 재검색이기도 해서 삭제하였습니다!

bb3cfcb

Comment on lines +34 to +54
Cypress.Commands.add("clickWatchedVideoListTab", () => {
cy.get("#watched-video-button").click();
});

Cypress.Commands.add("clickWatchLaterVideoListTab", () => {
cy.get("#watch-later-video-button").click();
});

Cypress.Commands.add("clickVideoWatchButton", () => {
cy.get(".video-item__watched-button").eq(0).click();
});

Cypress.Commands.add("clickVideoDeleteButton", (confirmButtonClick) => {
cy.get(".video-item__delete-button").eq(0).click();

cy.on("window:confirm", (text) => {
console.log(text);
expect(text).to.contains("정말로 삭제하시겠습니까?");
return confirmButtonClick;
});
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom Command 잘 사용해주셨어요 👏

Comment thread src/js/templates.js Outdated
Comment on lines +31 to +41
<img
src="${thumbnail}"
alt="video-item-thumbnail"
class="video-item__thumbnail"
/>
<h4 class="video-item__title">
${title}
</h4>
<p class="video-item__channel-name ">${channel}</p>
<p class="video-item__published-date ">${date}</p>
</a>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEM을 지켜주시려고 노력해주신게 보이네요... 🥺

Comment thread src/js/YoutubeApp.js
);
}

#reloadStorageData = () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

innerHTML을 하면 이전에 있던 비디오 정보들을 지우고 다시 렌더링을 할텐데 개선해볼 수 없을까요?
비디오를 보거나 삭제할 때, 검색 modal을 닫을 때 깜빡이는 현상이 있어 부자연스러워요.

필요한 데이터만 추가하고 삭제하는 방향으로 개선해보면 좋겠어요.

Copy link
Copy Markdown
Author

@usageness usageness Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존의 innerHTML로 되어있던 동작들을 찾아서 직접 지우고, 추가해주는 로직으로 모두 변경했습니다!

확인해보니 searchModal를 닫는 이벤트에서 메인 화면의 비디오도 비워버리는 의도치 않은 동작도 있었어요.
이것때문에 깜빡임 현상이 기존보다 더 눈에 띄는 면도 있었을것 같아요!

그럼에도 불구하고 깜빡임 현상이 완전히 사라지지는 않았는데, 썸네일 이미지를 외부에서 받아오다보니
요소가 갱신될때 해당 부분에서 일어나는 현상은 완벽히 개선되지는 않네요 😭😭
이런 경우에는 이미지 캐시를 따로 사용해야 할까요?

84faf8b

Copy link
Copy Markdown

@EungyuCho EungyuCho Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개선하느라 고생하셨어요 👏

이미지 캐시는 서버측해서 해줘야해요 😞
브라우저에서 컨트롤할만한 방법은 Blob으로 모두 저장해두는 방법도 있을거같긴한데 유의미할진 모르겠어요!.. 😅

Comment thread src/css/index.css
Comment on lines +54 to +81
@media (max-width: 1279px) {
#app {
max-width: 760px;
}

.search-modal {
max-width: 860px;
}
}

@media (max-width: 959px) {
#app {
max-width: 500px;
}

.search-modal {
max-width: 600px;
}
}

@media (max-width: 599px) {
#app {
max-width: 300px;
}

.search-modal {
max-width: 360px;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@usageness
Copy link
Copy Markdown
Author

안녕하세요, 아사님! 이번에도 꼼꼼한 피드백 정말 감사드려요 😊😊
하나씩 읽어보며 코멘트를 달고 있었는데 중간에 온 답변을 보고 감동했습니다 😭

피드백 해주셨던 부분 모두 수정했고, 완벽하지 않은 부분은 따로 코멘트로 남겼으니 확인 부탁드릴게요!
이번 일주일도 좋은 일만 가득하길 바랍니다!! 👍

Copy link
Copy Markdown

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

세지님 이번 유튜브 미션도 정말 고생하셨어요!

피드백 반영하신 부분 모두 확인했고 잘 고쳐주셨네요!

이전 step1에서 코멘트를 드린 줄 알았는데 놓친 부분이 있어 코드 코멘트에 한가지 남겨드렸어요.
앞으로도 timezone을 다루실 일이 있으실거라 생각해요. 그래서 링크드린 아티클은 정독해보시길 추천드릴게요! ㅎㅎㅎ

다음 미션인 자판기 미션만 하면 1단계 마무리이니 조금 더 힘내시길 바랄게요~
이번에도 세지님을 리뷰할 수 있어 좋았고 다음기회에 또 뵐게요!
승인하겠습니다!

channel: item.snippet.channelTitle,
thumbnail: item.snippet.thumbnails.high.url,
title: item.snippet.title,
date: parsedDate(item.snippet.publishTime),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsedDate 관련해서 이전 PR에서 코멘트를 드린줄알았는데 놓친줄 몰랐네요 💦

아티클을 읽어보고 표준시에 대해 알아보시면 좋을거같아요!
서버에서는 한국 시간이 아닌 국제 표준 시간으로 저장해줘서 해당 부분을 다룬 아티클인데 타임존을 다루실 때 많은 도움이 되실거에요 😄

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗.. 모두 적용했다고 생각했는데 놓친 부분이 있었네요ㅠㅠ
꼼꼼히 읽어보고 다음에 Date를 적용할땐 참고해야겠어요! 감사합니다 😊

@EungyuCho EungyuCho merged commit feb71ae into woowacourse:usageness Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants